feat(integration-test): use dapp-offer-up as target for getting-start… #8829

LuqiPan · 2024-01-26T21:53:31Z

…ed test

closes: #8381

Description

This PR uses dapp-offer-up in our CI job and (intends to) fix the tests associated with it

Testing Considerations

Gonna make integration tests pass again

LuqiPan · 2024-03-12T02:26:37Z

packages/agoric-cli/tools/getting-started.js

    );
    clearInterval(ival);
-
-    // Test that the Node.js `-r esm`-dependent plugin works.


remark

I don't know what these ESM plugin tests are about so I removed them

I think we can forget about resm-plugin

LuqiPan · 2024-03-12T02:27:58Z

packages/agoric-cli/tools/getting-started.js

-      detached: true,
-    });
+    // yarn start:ui
+    const uiStartP = yarn(['start:ui']);
    finalizers.push(() => pkill(uiStartP.childProcess, 'SIGINT'));


question

I don't know if I need to add finalizers for other yarn commands above?

turadg

Code LGTM.

I don't know the dapp-offer-up well enough to approve that this covers it so I'll leave Approve to @dckc

turadg · 2024-03-12T13:52:47Z

packages/agoric-cli/tools/getting-started.js

    );
    clearInterval(ival);
-
-    // Test that the Node.js `-r esm`-dependent plugin works.


I think we can forget about resm-plugin

turadg · 2024-03-12T13:54:39Z

packages/agoric-cli/tools/getting-started.js

-      clearTimeout(to);
-    };
+    // TODO: use abci_info endpoint to get block height
+    // sleep for 180 seconds for now


this comment will be stale if someone changes the constant.

Suggested change

// sleep for 180 seconds for now

// sleep to let contract start

("for now" is implied by the TODO)

turadg · 2024-03-12T13:55:52Z

packages/agoric-cli/tools/getting-started.js

-      t.is(done, 0, `deploy ${deployCmd.join(' ')} successful before timeout`);
-      clearTimeout(to);
-    };
+    // TODO: use abci_info endpoint to get block height


consider an XXX comment since this is tech debt, but not necessarily something we plan to fix. (unless you do)
https://github.com/Agoric/agoric-sdk/wiki/Coding-Style#code-comments

dckc

should start with yarn create ... rather than agoric init ...

dckc · 2024-03-12T14:34:53Z

packages/agoric-cli/tools/getting-started.js

-        ...initOptions,
-        'dapp-foo',
-      ]),
+      await myMain(['init', ...initOptions, 'dapp-foo']),


As of last Christmas, getting started no longer uses the agoric CLI.

This step should probably be yarn create @agoric/dapp --dapp-template dapp-offer-up dapp-foo

dckc · 2024-03-12T14:38:51Z

.github/workflows/integration.yml

@@ -57,7 +57,7 @@ jobs:
      # Prerequisites
      - uses: ./.github/actions/restore-node
        with:
-          node-version: 16.x
+          node-version: 18.18


It looks like the getting started test still clones agoric-sdk. That's no longer part of the workflow.

I think we still need to clone agoric-sdk, because with yarn create @agoric/dapp ... we're implicitly testing the behavior of init subcommand of agoric-cli package: https://github.com/Agoric/agoric-sdk/blob/master/packages/agoric-cli/src/main.js#L124-L140

Manual testing has confirmed that we don't need to clone agoric-sdk. yarn create @agoric/dapp gets code to run agoric init from npm, not from an agoric-sdk clone. I sure hope we can do likewise in ci.

This ci job says something about a local registry. That's perhaps the way yarn create @agoric/dapp ... gets access to the code under test.

This ci job says something about a local registry. That's perhaps the way yarn create @agoric/dapp ... gets access to the code under test.

Yeah I think we start a local NPM registry before running getting-started tests and we publish via a CI specific tag here.

...actually I'm not so sure anymore, just pushed a debug commit to weed out what's using local code and what's using npmjs.com

OK I just confirmed that yarn create @agoric/dapp ... will always pull from npmjs.com and I was not able to find a way to make it pull from local registry with my very limited yarn knowledge. I also confirmed that agoric init ... will use the local registry and code.

Here's what I propose:

in agoric-sdk repo, we use agoric init ... to initialize dapp-offer-up (this PR)

this way, we ensure integration tests in agoric-sdk repo are testing the behavior of agoric init ... at git HEAD, for example, if I change DEFAULT_DAPP_TEMPLATE to something erroneous, integration tests will fail right away

merging this PR would also unblock @turadg's PRs to deprecate support for node 16, add support for node 20, and migrate from yarn classic to yarn modern

in dapp-offer-up repo, we use yarn create @agoric/dapp ... to initialize dapp-offer-up followed by integration tests

this way, we ensure integration tests in dapp-offer-up repo are testing the behavior of agoric init ... that's published on npmjs.com

this work is not scheduled, and IMO is not high priority

@dckc sounds like a plan?

yes. sounds good.

dckc · 2024-03-12T17:12:14Z

packages/agoric-cli/tools/getting-started.js

    t.is(
-      await myMain(['init', ...initOptions, 'dapp-foo']),
+      await yarn(['create', '@agoric/dapp', 'dapp-foo']),


docs include --dapp-template dapp-offer-up options. Maybe those are no longer necessary?

Yeah it's no longer necessary because #8630 is merged in and released as part of upgrade-14

…ed test

…arted

dckc

onward

LuqiPan · 2024-03-13T00:49:22Z

Just realized that I meant to choose automerge:squash but somehow ended up choosing automerge:rebase... Oh well

LuqiPan added the force:integration Force integration tests to run on PR label Jan 26, 2024

0xpatrickdev mentioned this pull request Jan 29, 2024

drop Node 16 (EOL) #8365

Merged

LuqiPan force-pushed the 8381-integration-test-with-dapp-offer-up branch 2 times, most recently from 4e586f5 to e9623c8 Compare February 7, 2024 20:12

turadg mentioned this pull request Feb 14, 2024

integration tests with Node 20 #8900

Open

LuqiPan force-pushed the 8381-integration-test-with-dapp-offer-up branch 2 times, most recently from b739682 to e72b3b1 Compare February 14, 2024 00:32

LuqiPan force-pushed the 8381-integration-test-with-dapp-offer-up branch 2 times, most recently from aa2f3e7 to 2b940b8 Compare March 12, 2024 01:37

LuqiPan requested review from michaelfig, dckc and kriskowal March 12, 2024 02:24

LuqiPan marked this pull request as ready for review March 12, 2024 02:24

LuqiPan commented Mar 12, 2024

View reviewed changes

LuqiPan requested a review from turadg March 12, 2024 03:54

turadg reviewed Mar 12, 2024

View reviewed changes

dckc requested changes Mar 12, 2024

View reviewed changes

dckc reviewed Mar 12, 2024

View reviewed changes

LuqiPan force-pushed the 8381-integration-test-with-dapp-offer-up branch 3 times, most recently from e48e0b5 to d1d4d6d Compare March 12, 2024 19:32

LuqiPan added 7 commits March 12, 2024 14:14

feat(integration-test): use dapp-offer-up as target for getting-start…

e4f65ce

…ed test

fix(integration-test): bump node version from 16.x to 18.x

8f19ada

debug(integration-test): try node-version 18.18

a033926

debug(integration-test): comment out tests related to agoric deploy

a573b6f

fix(integration-test): update UI test case

c66fdc6

debug(integration-test): use 5173 as port number explicitly

184994c

fix(integration-test): remove another test that's based on testDeploy

5e5e348

LuqiPan added 8 commits March 12, 2024 14:14

chore: remove unused variable

210c4fe

ci(integration-test): run a3p container via service container

9e65b2c

test(integration-test): update the tests to follow current getting-st…

d687300

…arted

ci(integration-test): run yarn start:docker via getting-started.js

3a3711c

ci(integration-test): remove service container

ba76a1e

chore(integration-test): fix linting errors

5a4b29c

format(integration-test): let prettier run

93435bc

chore(integration-test): update comments and variable name

794d8fe

LuqiPan force-pushed the 8381-integration-test-with-dapp-offer-up branch from d1d4d6d to 794d8fe Compare March 12, 2024 21:14

dckc approved these changes Mar 12, 2024

View reviewed changes

dckc mentioned this pull request Mar 12, 2024

feat(getting-started): --dapp-template no longer needed Agoric/documentation#987

Merged

LuqiPan added the automerge:rebase Automatically rebase updates, then merge label Mar 13, 2024

mergify bot merged commit 7ba058a into master Mar 13, 2024
74 checks passed

mergify bot deleted the 8381-integration-test-with-dapp-offer-up branch March 13, 2024 00:31

LuqiPan mentioned this pull request Mar 13, 2024

ci for getting started with dapp-offer-up, docker-compose Agoric/dapp-offer-up#48

Closed

turadg mentioned this pull request Mar 13, 2024

drop tests for Node 16 #8638

Merged

mhofman mentioned this pull request Apr 19, 2024

Regression in integration test coverage for NPM packages #9258

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(integration-test): use dapp-offer-up as target for getting-start… #8829

feat(integration-test): use dapp-offer-up as target for getting-start… #8829

LuqiPan commented Jan 26, 2024 •

edited

Loading

LuqiPan Mar 12, 2024

turadg Mar 12, 2024

LuqiPan Mar 12, 2024

turadg left a comment

turadg Mar 12, 2024

turadg Mar 12, 2024

turadg Mar 12, 2024

dckc left a comment

dckc Mar 12, 2024

dckc Mar 12, 2024

LuqiPan Mar 12, 2024

dckc Mar 12, 2024

LuqiPan Mar 12, 2024

LuqiPan Mar 12, 2024

dckc Mar 12, 2024

dckc Mar 12, 2024

LuqiPan Mar 12, 2024

dckc left a comment

LuqiPan commented Mar 13, 2024

	// sleep for 180 seconds for now
	// sleep to let contract start

feat(integration-test): use dapp-offer-up as target for getting-start… #8829

feat(integration-test): use dapp-offer-up as target for getting-start… #8829

Conversation

LuqiPan commented Jan 26, 2024 • edited Loading

Description

Testing Considerations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

LuqiPan commented Mar 13, 2024

LuqiPan commented Jan 26, 2024 •

edited

Loading